[SVS] Implement 2-stage backend SVS index initialization#903
[SVS] Implement 2-stage backend SVS index initialization#903
Conversation
Purpose: prevent long time SVS Tiered index lock at initialization time. Logic: First stage: create `svs::...::MutableVamanaIndex` instance with R/O shared lock Second stage: set `svs::...::MutableVamanaIndex` created before under R/W unique lock
|
Hi, I’m Jit, a friendly security platform designed to help developers build secure applications from day zero with an MVS (Minimal viable security) mindset. In case there are security findings, they will be communicated to you as a comment inside the PR. Hope you’ll enjoy using Jit. Questions? Comments? Want to learn more? Get in touch with us. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #903 +/- ##
==========================================
- Coverage 97.09% 97.08% -0.01%
==========================================
Files 129 129
Lines 7493 7541 +48
==========================================
+ Hits 7275 7321 +46
- Misses 218 220 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
alonre24
left a comment
alonre24
left a comment
There was a problem hiding this comment.
LGTM!
A few small comments.
Also please:
- Validate that covering the affected scenarios in unit tests (I believe we are)
- Add micro benchmarks that will prove the improvement (add vector/run query while initial index creation is executed in the background)
| } | ||
|
|
||
| void setImpl(std::unique_ptr<ImplHandler> handler) override { | ||
| assert(handler && "SVSIndex::setImpl called with null handler"); |
There was a problem hiding this comment.
If this is a debug-only assert, let's add this to the log in a warning level as well
| std::span<const labelType> ids(labels, n); | ||
| auto processed_blob = this->preprocessForBatchStorage(vectors_data, n); | ||
| auto typed_vectors_data = static_cast<DataType *>(processed_blob.get()); | ||
| // Wrap data into SVS SimpleDataView for SVS API | ||
| auto points = svs::data::SimpleDataView<DataType>{typed_vectors_data, n, this->dim}; | ||
|
|
There was a problem hiding this comment.
This logic seems to be a duplication of what we do in addVectorsImpl. Consider unifying these into a single function
| SVSImplHandler *svs_handler = dynamic_cast<SVSImplHandler *>(handler.get()); | ||
| if (!svs_handler) { | ||
| throw std::logic_error("Failed to cast to SVSImplHandler"); | ||
| } |
There was a problem hiding this comment.
What is the motivation to have an abstract ImplHandler rather than have only SVSImplHandler? The dynamic_cast here seems a bit awkward
| svs_index->setNumThreads(std::min(availableThreads, labels_to_move.size())); | ||
| svs_index->addVectors(vectors_to_move.data(), labels_to_move.data(), | ||
| labels_to_move.size()); | ||
| if (this->backendIndex->indexSize() == 0) { |
There was a problem hiding this comment.
Does this also handle re-initialization after the index was emptied? Is that scenario tested?
3 participants
Purpose: prevent long time SVS Tiered index lock at initialization time.
Logic:
svs::...::MutableVamanaIndexinstance with R/O shared locksvs::...::MutableVamanaIndexcreated before under R/W unique lockWhich issues this PR fixes
Main objects this PR modified
createImpl()andsetImpl()createImpl()under shared lock andsetImpl()under unique lock if backend index is empty.Mark if applicable